Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add write roundtrip test #8169

Merged
merged 2 commits into from
Jul 9, 2021
Merged

Add write roundtrip test #8169

merged 2 commits into from
Jul 9, 2021

Conversation

bzbarsky-apple
Copy link
Contributor

Problem

We don't have a unit test for a round-trip write: instantiate a WriteClient, do a write on it, have that create a WriteHandler on the destination, respond, etc.

Change overview

Adds such a test.

Testing

Ran the unit tests. This uncovered a few bugs in our implementation that are fixed as part of the PR.

Reviewer note

Reviewing the two changesets in this PR separately might be simpler than reviewing the overall diff.

@bzbarsky-apple
Refactor TestWriteInteraction to use a MessagingContext.
bb463c9 
This allows us to test more details of the messages being sent.
@boring-cyborg boring-cyborg bot added the app label Jul 7, 2021
yunhanw-google
yunhanw-google approved these changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@yunhanw-google yunhanw-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@bzbarsky-apple
Add a roundtrip test to TestWriteInteraction.
a9de05b 
Various fixups needed for that:

1) Make InteractionModelEngine::Shutdown actually remove the object as
   an unsolicited message handler.

2) Initialize WriteHandler's state properly so it's not random whether
   we think we have available WriteHandlers.

3) Fix TestWriteInteraction::AddAttributeDataElement to use the right
   tag (2, not 1) for its data.

4) Stop trying to run AppTests in QEMU, because of linking/compilation issues
   in that setup.
@bzbarsky-apple bzbarsky-apple force-pushed the add-write-roundtrip-test branch from dc84b2e to a9de05b Compare July 9, 2021 04:03
@boring-cyborg boring-cyborg bot added the scripts label Jul 9, 2021
@todo
Copy link

todo bot commented Jul 9, 2021

libAppTests depends on MessagingTestHelpers, which depends on

connectedhomeip/scripts/tests/esp32_qemu_tests.sh

Lines 64 to 74 in a9de05b

# TODO: libAppTests depends on MessagingTestHelpers, which depends on
# NetworkTestHelpers. That sort of depends on InetTestHelpers or
# equivalent (to provide gSystemLayer, gInet, InitNetwork(),
# ShutdownNetwork()) but there's only a POSIX implementation of that
# last, which does not compile on ESP32. Need to figure out how to
# make that work. See comments below for the transport layer tests,
# which have the same issue.
# run_suite libAppTests.a -lMessagingTestHelpers -lNetworkTestHelpers
run_suite libASN1Tests.a
run_suite libBleLayerTests.a

This comment was generated by todo based on a TODO comment in a9de05b in #8169. cc @bzbarsky-apple.

@github-actions
Copy link

github-actions bot commented Jul 9, 2021

Size increase report for "esp32-example-build" from 046f46a

File Section File VM
chip-lock-app.elf .flash.text -64 -64
Full report output 
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
[Unmapped],0,64
.debug_info,0,21
.debug_abbrev,0,12
.debug_line,0,10
.debug_str,0,2
.debug_ranges,0,-8
.flash.text,-64,-64
.debug_loc,0,-177

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_info,0,20
.debug_abbrev,0,12
.debug_line,0,10
.debug_str,0,2
.debug_ranges,0,-8
.debug_loc,0,-184

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,20
.debug_abbrev,0,12
.debug_line,0,10
.debug_str,0,-2
.debug_ranges,0,-8
.debug_loc,0,-184

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

@bzbarsky-apple
Copy link
Contributor Author

@msandstedt @saurabhst Please take a look?

@bzbarsky-apple bzbarsky-apple merged commit 26e1a06 into project-chip:master Jul 9, 2021
@bzbarsky-apple bzbarsky-apple deleted the add-write-roundtrip-test branch July 9, 2021 16:08
@todo todo bot mentioned this pull request Jul 9, 2021
Closed
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
@bzbarsky-apple
Add write roundtrip test (project-chip#8169)
5e23ec5 
* Refactor TestWriteInteraction to use a MessagingContext.

This allows us to test more details of the messages being sent.

* Add a roundtrip test to TestWriteInteraction.

Various fixups needed for that:

1) Make InteractionModelEngine::Shutdown actually remove the object as
   an unsolicited message handler.

2) Initialize WriteHandler's state properly so it's not random whether
   we think we have available WriteHandlers.

3) Fix TestWriteInteraction::AddAttributeDataElement to use the right
   tag (2, not 1) for its data.

4) Stop trying to run AppTests in QEMU, because of linking/compilation issues
   in that setup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmartinez-silabs jmartinez-silabs jmartinez-silabs approved these changes

@yunhanw-google yunhanw-google yunhanw-google approved these changes

@saurabhst saurabhst saurabhst approved these changes

@andy31415 andy31415 andy31415 approved these changes

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@jelderton jelderton Awaiting requested review from jelderton

@LuDuda LuDuda Awaiting requested review from LuDuda

@mspang mspang Awaiting requested review from mspang

@selissia selissia Awaiting requested review from selissia

@woody-apple woody-apple Awaiting requested review from woody-apple

Assignees
No one assigned
Labels
app review - approved scripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bzbarsky-apple @andy31415 @saurabhst @yunhanw-google @jmartinez-silabs